Allow configuring cascade deletion in DABs#5846
Conversation
Add a `cascade` field to the pipeline bundle resource that controls whether destroying a pipeline also deletes its datasets (materialized views, streaming tables, and views). When unset, the server default applies (cascade); set `cascade: false` to retain the datasets on destroy. The field is delete-time only: it is not part of the pipeline spec, so it is never sent on create/update. The direct engine persists it in state (via a PipelineState wrapper around CreatePipeline, mirroring the sql_warehouse lifecycle pattern) and reads it at delete time, force-sending cascade so an explicit false survives query-string omitempty (honored for query params as of databricks-sdk-go v0.152.0). A cascade-only change is a state-only update with no pipeline API call. cascade is classified input_only so it does not show remote drift. The terraform engine drops the field for now: the pinned provider has no such attribute yet (pending terraform-provider-databricks#5860), so terraform-engine support is a follow-up. Co-authored-by: Isaac
Approval status: pending
|
| pipelines: | ||
| my_pipeline: | ||
| name: test-pipeline-cascade | ||
| cascade: false |
There was a problem hiding this comment.
Could you make sure the default template also sets purge_on_destroy: false? With a very short comment to the side explaining it? Could be a separate PR.
There was a problem hiding this comment.
ack will put this change in a follow-up PR.
… message fix - Rename the pipeline config field cascade -> cascade_on_destroy, matching the Lakebase purge_on_delete precedent (per review discussion). - Add ValidateCascadeOnDestroy mutator: hard error when the field is used with the terraform engine, which does not support it yet. - Fix the destroy approval message so a pipeline with cascade_on_destroy: false is not described as deleting its STs/MVs, and mention the property otherwise. - Comment why ForceSendFields is needed in DoDelete. - Regenerate schema, refschema, and affected acceptance outputs. Co-authored-by: Isaac
| pipelines: | ||
| my_pipeline: | ||
| name: test-pipeline-cascade | ||
| cascade: false |
There was a problem hiding this comment.
ack will put this change in a follow-up PR.
…cade # Conflicts: # NEXT_CHANGELOG.md
Align the pipeline destroy-time attribute name with the DABs config field (databricks/cli#5846), which settled on cascade_on_destroy. Co-authored-by: Isaac
|
|
||
| This action will result in the deletion of the following Lakeflow Spark Declarative Pipelines along with the | ||
| Streaming Tables (STs) and Materialized Views (MVs) managed by them: | ||
| Streaming Tables (STs) and Materialized Views (MVs) managed by them. Set 'cascade_on_destroy: false' on a pipeline to retain datasets on pipeline deletion: |
There was a problem hiding this comment.
| Streaming Tables (STs) and Materialized Views (MVs) managed by them. Set 'cascade_on_destroy: false' on a pipeline to retain datasets on pipeline deletion: | |
| Streaming Tables (STs) and Materialized Views (MVs) managed by them. Set 'cascade_on_destroy: false' on a pipeline to retain the Streaming Tables and Materialized Views on pipeline deletion: |
datasets seems like a vague term to use here? In the context of dashboards, for example, this just means a SQL query.
I could be wrong about this, so if this is standard for pipelines, feel free to keep it as is.
There was a problem hiding this comment.
We generally use datasets in pipelines docs now, since pipeline-owned tables have been changing recently. Now, we also allow users to create Views in pipelines. In the near future, we plan to switch to creating regular delta tables as well.
| type PipelineRemote struct { | ||
| pipelines.CreatePipeline | ||
|
|
||
| CascadeOnDestroy *bool `json:"cascade_on_destroy,omitempty"` |
There was a problem hiding this comment.
Can be omitted from this struct because cascade on destroy will never be set in the remote. It'll never be returned as part of the get response
There was a problem hiding this comment.
ah I was confused and thought that there was a strict requirement that PipelineRemote be a superset. But looking closer, I don't think there a benefit to defining for the remote version. Dropped it
| deletePipelineWithCascadeMessage = `This action will result in the deletion of the following Lakeflow Spark Declarative Pipelines along with the | ||
| Streaming Tables (STs) and Materialized Views (MVs) managed by them. Set 'cascade_on_destroy: false' on a pipeline to retain datasets on pipeline deletion:` | ||
|
|
||
| deletePipelineNoCascadeMessage = `This action will result in the deletion of the following Lakeflow Spark Declarative Pipelines. |
Drop the files.watcherExclude block that was unrelated to the cascade_on_destroy feature, per review feedback. Co-authored-by: Isaac
- Add docs URL to the terraform-engine validation error so users can learn about direct deployment mode. - Omit CascadeOnDestroy from PipelineRemote: it is an input-only, delete-time setting the GET response never returns, so drift is suppressed automatically (missing_in_remote). Matches the postgres input-only field pattern. - Propagate errors from pipelineDeletionCascades instead of silently defaulting, and add unit coverage. Co-authored-by: Isaac
|
An authorized user can trigger integration tests manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
|
|
||
| v, err := dyn.GetByPath(b.Config.Value(), path) | ||
| if err != nil { | ||
| return true, nil |
There was a problem hiding this comment.
This embeds a client side default here. Treating unset as true. Won't this get out of date once the server side default changes?
There was a problem hiding this comment.
I guess there's no good way to handle this scenario? It does mean we'll eventually end up showing wrong messages to people.
There was a problem hiding this comment.
Can you add a to-do message here to change the value once the server side default flips?
There was a problem hiding this comment.
Yep, added a note here. Unfortunately, there may be a brief period where the DABs default and server-side default are different. But the DABs documentation describes the actual encoded behavior, so we won't be showing the wrong messages
|
|
||
| v, err := dyn.GetByPath(b.Config.Value(), path) | ||
| if err != nil { | ||
| return true, nil |
There was a problem hiding this comment.
Can you add a to-do message here to change the value once the server side default flips?
| deletePipelineWithCascadeMessage = `This action will result in the deletion of the following Lakeflow Spark Declarative Pipelines along with the | ||
| Streaming Tables (STs) and Materialized Views (MVs) managed by them. Set 'cascade_on_destroy: false' on a pipeline to retain datasets on pipeline deletion:` | ||
|
|
||
| deletePipelineNoCascadeMessage = `This action will result in the deletion of the following Lakeflow Spark Declarative Pipelines. |
There was a problem hiding this comment.
GPT 5.6 flagged a crucial bug. @ronaldz-db we need to read the cascade_on_delete value from state when showing the prompt. Otherwise we risk deleting a user's table when tell them that we'll not:
Critical — approval can falsely promise dataset retention. /tmp/isaac-review-cli/bundle/phases/plan.go:46 reads the current YAML, while deletion uses persisted state.
If a user changes cascade_on_destroy to false and immediately destroys without deploying, the prompt says datasets are retained, but the API uses the old state and
cascades. Derive both behavior and messaging from the same state; add regression coverage for this sequence.
Focused package tests pass. Current CI failures are unrelated JFrog OIDC setup failures.
REQUEST CHANGES
There was a problem hiding this comment.
Can you check if this feedback is valid? I suspect it is because from I recall we indeed only rely on the state for delete / destroy operations.
Please also add an acceptance test to ensure that we do the right thing when a user sets this field but then immediately destroys the bundle the bundle without a deployment.
There was a problem hiding this comment.
oh shoot, thanks for the catch. This feedback is valid. I've updated the message to be generated by reading the state value for the direct engine and added a test.
Changes
Add a
cascade_on_destoryfield to the pipeline bundle resource that controls whether destroying a pipeline also cascades to its datasets. When unset, we maintain the existing default of true.This change applies to the direct engine. We have a separate change on the Terraform side: databricks/terraform-provider-databricks#5860
Why
Previously, pipeline deletion would automatically cascade to its tables. After user feedback, we decided to make this configurable in the DeletePipeline API. This extends support to DABs as well, which was another common user ask.
Tests
Unit tests